Handle firefox flatpak installation #1597
Conversation
elken
force-pushed
the
feat/firefox-flatpak
branch
from
210e265
to
4f06948
Compare
|
Hi, thanks a lot for taking the time to add Flatpak support to Firenvim! Really appreciated :). I think your pull request should work as is, but I wonder if it might be better to treat a firefox flatpak installation as a separate browser, i.e. instead of modifying the definition of the existing "firefox" installation, we would have: diff --git a/autoload/firenvim.vim b/autoload/firenvim.vim
index 7311c50..fe9a9e3 100644
--- a/autoload/firenvim.vim
+++ b/autoload/firenvim.vim
@@ -770,7 +770,12 @@ function! s:get_browser_configuration() abort
\ 'manifest_content': function('s:get_firefox_manifest'),
\ 'manifest_dir_path': function('s:get_firefox_manifest_dir_path'),
\ 'registry_key': 'HKCU:\Software\Mozilla\NativeMessagingHosts\firenvim',
- \ 'flatpak_name': 'org.mozilla.firefox',
+ \},
+ \'firefox_flatpak': {
+ \ 'has_config': s:firefox_flatpak_config_exists(),
+ \ 'manifest_content': function('s:get_firefox_manifest'),
+ \ 'manifest_dir_path': function('s:get_firefox_flatpak_manifest_dir_path'),
+ \ 'registry_key': 'HKCU:\Software\Mozilla\NativeMessagingHosts\firenvim',
\},
\'librewolf': {
\ 'has_config': s:librewolf_config_exists(),The reason I'm thinking this might be better is that as far as I can tell, a "regular" firefox installation and a flatpak one can coexist on a same computer. I'm also under the impression that it might make implementing flatpak-specific behavior easier. What do you think about this? If you think this might indeed be better, do you want to work on it or would you prefer that I do it? :) |
|
So the reason I went with the approach I did was it makes it easier to later add support for flatpaks for the other browsers without having to add a new browser type for each one (also snap, etc) but I don't have any inherent problem with it being implemented that way 😄 It sounds like you're otherwise happy with the rest, so I'll make that change & add some documentation/troubleshooting info and then this should be good to go if you're happy |
|
Apologies for not getting to this, I've had a pretty busy week and I've got another one but I've planned some time at the weekend to look at getting this done 😄 |
|
No problem! I can get quite busy too at times, take all the time you need :) |
elken
force-pushed
the
feat/firefox-flatpak
branch
from
4f06948
to
393cfbd
Compare
|
I think that looks fine docs wise, is there anything else you think I should add? 😄 |
There was a problem hiding this comment.
Thank you for implementing these changes and documenting installation instructions! I apologize in advance but I am going to need to spend more time thinking about your PR before merging it, I'm perfectly happy to have Firenvim's flatpak support break flatpak's sandbox, but I think this needs to be communicated extremely clearly to the user and in a way that cannot be ignored. I'm not sure what the best way to do this is yet.
|
|
||
| ```conf | ||
| [Context] | ||
| filesystems=/run/user/1000/firenvim;~/.local/share/firenvim;~/.local/share/nvim;~/.config/nvim |
There was a problem hiding this comment.
Just thinking: this effectively breaks flatpak's sandboxing. A compromised firefox will be able to append malicious code to the user's init.lua, which will be executed outside the sandbox the next time the user runs neovim from their terminal.
There was a problem hiding this comment.
The only way around it would be to copy the directories instead of symlinking them, but then you'd have to re-run it any time your config changed or you updated a plugin.
I have pondered that perhaps this PR would be better as purely documentation rather than any changes the plugin makes, then the setup is up to the user.
|
|
||
| call s:maybe_execute('system', ['ln', '-nsf', stdpath("config"), s:build_path([$HOME, '.var', 'app', l:cur_browser['flatpak_name'], "config", "nvim"])]) | ||
| call s:maybe_execute('system', ['ln', '-nsf', stdpath("data"), s:build_path([$HOME, '.var', 'app', l:cur_browser['flatpak_name'], "data", "nvim"])]) | ||
| else |
There was a problem hiding this comment.
It seems that most of the code in the if l:flatpak_result branch is shared with the code in the else branch. What do you think about having a single execution path and only gating the two system calls behind an if l:flatpak_result? e.g.
endtry
call s:maybe_execute('mkdir', l:manifest_dir_path, 'p', 0700)
call s:maybe_execute('writefile', [l:manifest_content], l:manifest_path)
call s:maybe_execute('setfperm', l:manifest_path, 'rw-------')
echo 'Installed manifest for ' . l:name . '.'
if s:browser_has_flatpak_installed(l:cur_browser)
call s:maybe_execute('system', ['ln', '-nsf', stdpath("config"), s:build_path([$HOME, '.var', 'app', l:cur_browser['flatpak_name'], "config", "nvim"])])
call s:maybe_execute('system', ['ln', '-nsf', stdpath("data"), s:build_path([$HOME, '.var', 'app', l:cur_browser['flatpak_name'], "data", "nvim"])])
endif
if has('win32') || s:is_wsl
...There was a problem hiding this comment.
That does actually make the most sense yes
Summary
This should take care of a number of issues w.r.t installing this
within a flatpak sandboxed browser.
At present, this is only tested against firefox but there's little reason it
shouldn't work the same for other browsers.
There are also a few more paths that need to be overridden in that flatpak, namely:
stdpath("config")stdpath("data")~/.local/share/firenvim/run/user/1000/firenvimWhich will be included in the documentation, as well as added to troubleshooting steps.
What this PR changes
If a browser configuration has a key denoting its flatpak name, then we also
check if there is a flatpak installed. If not, we move on. If there is no
flatpak installed, we also move on.
The interesting thing we now do here is creating symbolic links from your
config inside the flatpak's config folder and your data path inside the
flatpak's data folder. Doing that is the only check I needed to get this
working (as I write this issue now inside firenvim)
What's here has been tested locally by me insofar as I can, but I would be much
more comfortable if others tested this as well. Others testing this might also
demonstrate how simply this can be applied to other browsers like Chrome; which
I don't use but I can spend some time testing those if you'd prefer every
browser to be in this PR.
If it's not also evident from the code, I haven't touched VimScript in nearly a
decade ... so feel free to correct my code as you see fit.
I know there's also no documentation; to save me constantly updating it I'll
wait until you're happy with this PR & add some in then. I think there's a few
more troubleshooting steps I can add in too.
Thanks for a fantastic tool!